TyKind in GlobalAllocs
#84
Open
+217
−203
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jberthold
reviewed
May 21, 2025
There was a problem hiding this comment.
Looks good! And makes sense to add the type information at this stage.
Two points to consider changing, however:
- The PR proposes to store
TyKind(which should always beRigidTyBTW) but the extraction generally (and the semantics so far) instead stores and uses theTyIDs to refer to a type. By storingRigidTyhere we have to engage in decoding theRigidTyto something useful. - The "thing that's useful" already exists in the code as
TypeInfo, so we would be duplicating the decoder.
I'd suggest we just store the Ty ID with the alloc instead of the RigidTy and use the lookups into the existing types table and existing decoder code. The change would be relatively easy: At the call site for collect_alloc we can pass in the ty and get the resp. TyKind within collect_alloc.
src/printer.rs
Outdated
| if debug_enabled() { | ||
| println!( | ||
| "DEBUG: called collect_alloc: {:?}:{:?}:{:?}", | ||
| val, pointed_kind, global_alloc | ||
| ); | ||
| } | ||
| entry.or_insert(AllocInfo::Memory(alloc.clone())); // TODO: include pointed_kind.clone().unwrap() ? | ||
| entry.or_insert((kind.unwrap(), AllocInfo::Memory(alloc.clone()))); // TODO: include pointed_kind.clone().unwrap() ? |
There was a problem hiding this comment.
Suggested change
| entry.or_insert((kind.unwrap(), AllocInfo::Memory(alloc.clone()))); // TODO: include pointed_kind.clone().unwrap() ? | |
| entry.or_insert((kind.unwrap(), AllocInfo::Memory(alloc.clone()))); |
There was a problem hiding this comment.
Totally agree! Amending now!
| { | ||
| "kind": "ReErased" | ||
| }, | ||
| 31, |
There was a problem hiding this comment.
This is the problem that fails the golden tests in CI, the IDs stored here are unstable.
ehildenb
approved these changes
May 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
GlobalAllocs need to be decoded to their types, this can be done easier by preprocessing the python (maybe in the future in the repo). But for the python to preprocess it will go easier with theTyavailable in the JSON.AllocInfoto be a struct that contains theGlobalAllocinformation, originally it was a enum that was a duplicate ofstable_mir::mir::alloc::GlobalAlloc. The struct helps with the JSON as the field names come through.TyKindfrom the allocs and provs to work withTycollect_allocalso takes aTyas a parameter instead of aTyKindnowAllocMapis now collects theTyalso